Update the docs for Google pub/sub #564
Conversation
Signed-off-by: Somtochi Onyekwere <[email protected]>
Signed-off-by: Somtochi Onyekwere <[email protected]>
There was a problem hiding this comment.
Ouch, thanks for catching this @somtochiama!
Looks like the only way to make this provider work right now is by not specifying spec.address (which is optional, despite the ^(http|https)://.*$ validation), and specifying the address on a secret, as you said. Thanks for fixing these docs!
I'd definitely vote on lifting the URL validation for this field from the CRD, looks like it doesn't make sense anymore. By the way, even though right now this field is optional in the CRD, the event handler in fact errors out and skips the alert in case both this field and the address field on the referenced secret are empty:
notification-controller/internal/server/event_handlers.go
Lines 222 to 225 in 4a90beb
Hence the address field is mandatory in practice. Setting spec.address to http:// (or https://) and the real address in the secret would be super weird.
|
@somtochiama I opened #565 for lifting the HTTP/S validation. In case it's approved/merged, I think we don't need to update the docs like this anymore |
|
Closing in favour of #565 |
This pull request updates the Google pub/sub documentation to have the address in the referenced Provider secret instead of the spec. This is because the address for Google Pub/Sub is the project-id and fails the URL validation for the field.
We should also reconsider the validation for the
spec.addressfield and how the current validation prevents some values from being specified in the spec. Some Providers (e.g. Azure EventHub) have address values that wouldn't pass the validation and use theaddressfield instead of the secret instead. But this can come up after there has been some discussion around possible solutions.